internal/controller/flp/flp_controller_test.go (1)

173-174: Use informerName constant instead of string literal

Replace hardcoded "flp-informers" in expectations with informerName to avoid drift if naming changes.

Also applies to: 336-337

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_controller_test.go` around lines 173 - 174,
Replace the hardcoded string "flp-informers" in the test expectations with the
existing informerName constant so the assertions use informerName instead of a
literal; update the checks that reference rb3.Subjects[0].Name and any other
assertions (e.g., the ones at the other occurrence around lines noted) to
compare to informerName, leaving the RoleRef check (rb3.RoleRef.Name) as-is
unless it should also be replaced by a corresponding constant.

internal/controller/flp/flp_informer_builder.go (1)

60-69: Consider making informer resources tunable

Hardcoding informer resources may under-size large clusters. Consider deriving from config/CR defaults so limits can be adjusted without code changes.

As per coding guidelines "Monitor memory usage impact for large scale clusters - watch for memory growth with large flow volumes and optimize cacheMaxFlows, cacheActiveTimeout, writeBatchWait, writeBatchSize settings".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_builder.go` around lines 60 - 69, The
ResourceRequirements for the informer are hardcoded (corev1.ResourceRequirements
with fixed Requests/Limits); make these values configurable by reading from the
controller config or CR defaults instead of literals—e.g., add config/CR fields
(or reuse existing controller config) for informer.memoryRequest,
informer.cpuRequest, informer.memoryLimit, informer.cpuLimit, and use them when
constructing the ResourceRequirements in flp_informer_builder.go (fall back to
the current "128Mi"/"50m"/"256Mi"/"200m" defaults if unset); ensure the
builder/constructor that creates the informer (the code that sets
ResourceRequirements) accepts or fetches the config/CR and applies those values
so operators can tune
cacheMaxFlows/cacheActiveTimeout/writeBatchWait/writeBatchSize and informer
resource limits without code changes.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/controller/flp/flp_informer_reconciler.go (1)

60-64: Consider handling TryDeleteAll errors.

TryDeleteAll return value is ignored. While "Try" implies best-effort, logging failures would aid debugging cleanup issues.

Suggested improvement

 	if desired.Spec.OnHold() {
 		r.Status.SetUnused("FlowCollector is on hold")
-		r.Managed.TryDeleteAll(ctx)
+		if err := r.Managed.TryDeleteAll(ctx); err != nil {
+			log.FromContext(ctx).Error(err, "failed to delete some managed resources")
+		}
 		return nil
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_reconciler.go` around lines 60 - 64,
When handling the OnHold branch, capture the error returned by
r.Managed.TryDeleteAll(ctx) instead of ignoring it; call it into a variable
(e.g., err := r.Managed.TryDeleteAll(ctx)) and if non-nil emit a diagnostic (use
the reconciler logger or status updater such as r.Log.Error(err, "failed to
cleanup managed resources on hold") or r.Status.SetError/SetDegraded with
context) so cleanup failures are recorded, then continue returning nil as
before.

internal/controller/flp/flp_informer_builder.go (3)

60-68: Hardcoded resource limits.

Resource requirements are fixed at 128Mi/256Mi memory and 50m/200m CPU. Other FLP components use desired.Processor.Resources from the CR spec. Consider exposing informer resources via config or document why fixed values are appropriate here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_builder.go` around lines 60 - 68, The
informer is using hardcoded resource values in the Resources block; replace the
fixed corev1.ResourceRequirements with the FLP spec's resource settings (use
desired.Processor.Resources) or thread a configurable field through the informer
builder so callers can supply resource requirements; update the builder function
that constructs the informer pod (look for Resources:
corev1.ResourceRequirements in flp_informer_builder.go) to read and apply
desired.Processor.Resources (or a new InformerResources field) and ensure
fallback/default values are documented or applied when the CR value is nil.

---

90-96: Pod template missing part-of label.

Deployment ObjectMeta.Labels includes "part-of": constants.OperatorName but the pod template labels don't. Other FLP builders (monolith, transformer) include this label in their pod templates for consistency.

Suggested fix

 			Template: corev1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{
+						"part-of": constants.OperatorName,
 						"app":     informerName,
 						"version": version,
 					},
 				},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_builder.go` around lines 90 - 96, The
pod template under Template: corev1.PodTemplateSpec for the FLP informer builder
is missing the "part-of" label; update the ObjectMeta.Labels map inside the
PodTemplateSpec (the block that currently sets "app": informerName and
"version": version) to also include "part-of": constants.OperatorName so the pod
template matches the Deployment ObjectMeta.Labels and other FLP builders
(monolith, transformer).

---

29-104: Method signature returns error but never fails.

deployment() returns (*appsv1.Deployment, error) but always returns nil error. Either simplify the signature to return only *appsv1.Deployment, or add validation that could fail.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_builder.go` around lines 29 - 104, The
deployment() method on informerBuilder currently returns (*appsv1.Deployment,
error) but never returns a non-nil error; either remove the unused error return
or add meaningful validation. To fix quickly, change informerBuilder.deployment
signature to return *appsv1.Deployment (no error), update its return to only the
Deployment value, and update all callers to accept the single return (search for
calls to deployment() and adjust unpacking). Alternatively, if you prefer
validation, add checks inside deployment (e.g., ensure
b.Images[reconcilers.MainImage] is non-empty and b.desired is non-nil) and
return an error when invalid instead of always nil; update callers accordingly
to handle the error.

internal/controller/flp/flp_monolith_objects.go (1)

183-188: Use constant and consistent type for k8scache port.

Line 185 hardcodes 9090 and line 187 uses intstr.FromInt() instead of intstr.FromInt32(). Use the k8scachePort constant for maintainability and match the int32 pattern used on line 181.

Suggested fix

 			{
 				Name:       "k8scache",
-				Port:       9090,
+				Port:       k8scachePort,
 				Protocol:   corev1.ProtocolTCP,
-				TargetPort: intstr.FromInt(9090),
+				TargetPort: intstr.FromInt32(k8scachePort),
 			},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_monolith_objects.go` around lines 183 - 188,
Replace the hardcoded 9090 and the intstr.FromInt call with the k8scachePort
constant and an int32 conversion so types are consistent: set Port to
k8scachePort and TargetPort to intstr.FromInt32(int32(k8scachePort)) for the
object with Name "k8scache" (the same pattern used on the earlier port
definition).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

config/crd/bases/flows.netobserv.io_flowcollectors.yaml (1)

5150-5157: ⚠️ Potential issue | 🟡 Minor

replicas validation contradicts the documented requirement.

minimum: 1 conflicts with the field description saying 2 is required when enabled=true. Please either enforce the conditional constraint or reword it as a recommendation.

Suggested schema-side enforcement

                     informers:
                       description: |-
                         `informers` configuration for centralized Kubernetes informers that push cache updates to flowlogs-pipeline processors.
                         This reduces load on the Kubernetes API server by having a single component (flp-informers) query the API instead of N FLP processors.
                         When enabled, a dedicated `flp-informers` deployment is created that watches Kubernetes resources and pushes updates via gRPC.
                       properties:
@@
                         replicas:
                           default: 2
                           description: |-
                             `replicas` defines the number of replicas for the flp-informers deployment.
                             For high availability, a minimum of 2 replicas is required when `enabled` is `true`.
                           format: int32
                           minimum: 1
                           type: integer
@@
+                      x-kubernetes-validations:
+                        - message: "informers.replicas must be >= 2 when informers.enabled is true."
+                          rule: "!has(self.enabled) || !self.enabled || !has(self.replicas) || self.replicas >= 2"
                       type: object

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@config/crd/bases/flows.netobserv.io_flowcollectors.yaml` around lines 5150 -
5157, The replicas schema for the flp-informers deployment currently declares
minimum: 1 but the description mandates "a minimum of 2 replicas when `enabled`
is `true` — fix this by either updating the schema to enforce the constraint or
rewording the description: either change the `replicas` validation so minimum: 2
and/or add an if/then conditional that checks the `enabled` boolean and requires
replicas >= 2 when `enabled: true`, or keep minimum: 1 but rephrase the
description to a recommendation (e.g., "recommended minimum of 2 when `enabled`
is true"); locate the `replicas` field and the related `enabled` flag in the
flowcollectors CRD to apply the chosen fix.

internal/controller/flp/flp_common_objects.go (3)

98-124: Extract the port-name literal and simplify the repetition.

Four near-identical branches with the same error template. A small table collapses this nicely and makes future additions (e.g., another reserved port) one-line changes:

♻️ Suggested refactor

-func validatePortConflicts(desired *flowslatest.FlowCollectorSpec) error {
-	advancedConfig := helper.GetAdvancedProcessorConfig(desired)
-
-	// Check FLP port
-	if advancedConfig.Port != nil && *advancedConfig.Port == k8scachePort {
-		return fmt.Errorf("flowlogs-pipeline port %d conflicts with reserved k8scache port %d", *advancedConfig.Port, k8scachePort)
-	}
-
-	// Check health port
-	if advancedConfig.HealthPort != nil && *advancedConfig.HealthPort == k8scachePort {
-		return fmt.Errorf("flowlogs-pipeline health port %d conflicts with reserved k8scache port %d", *advancedConfig.HealthPort, k8scachePort)
-	}
-
-	// Check metrics port
-	metricsPort := desired.Processor.GetMetricsPort()
-	if metricsPort == k8scachePort {
-		return fmt.Errorf("flowlogs-pipeline metrics port %d conflicts with reserved k8scache port %d", metricsPort, k8scachePort)
-	}
-
-	// Check profile port (optional)
-	if advancedConfig.ProfilePort != nil && *advancedConfig.ProfilePort == k8scachePort {
-		return fmt.Errorf("flowlogs-pipeline profile port %d conflicts with reserved k8scache port %d", *advancedConfig.ProfilePort, k8scachePort)
-	}
-
-	return nil
-}
+func validatePortConflicts(desired *flowslatest.FlowCollectorSpec) error {
+	adv := helper.GetAdvancedProcessorConfig(desired)
+	checks := []struct {
+		label string
+		port  *int32
+	}{
+		{"port", adv.Port},
+		{"health port", adv.HealthPort},
+		{"metrics port", ptr.To(desired.Processor.GetMetricsPort())},
+		{"profile port", adv.ProfilePort},
+	}
+	for _, c := range checks {
+		if c.port != nil && *c.port == k8scachePort {
+			return fmt.Errorf("flowlogs-pipeline %s %d conflicts with reserved k8scache port %d", c.label, *c.port, k8scachePort)
+		}
+	}
+	return nil
+}

Also — this is a spec validation and arguably belongs next to validateFLP() in the webhook, not inside the builder. Builders today silently swallow the error in tests (b, _ := newMonolithBuilder(...)), so a misconfig would surface as a broken controller reconcile rather than a clean webhook rejection.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_common_objects.go` around lines 98 - 124, The
validatePortConflicts function repeats the same error template for multiple
ports; refactor it by extracting the error message template and building a small
map/slice of (portName, portValue) entries using advancedConfig.Port,
advancedConfig.HealthPort, desired.Processor.GetMetricsPort(), and
advancedConfig.ProfilePort, then iterate and return fmt.Errorf(fmtTemplate, val,
k8scachePort) when a conflict is found; update references to the unique symbols
validatePortConflicts, advancedConfig, desired.Processor.GetMetricsPort, and
k8scachePort to implement the loop-based check and remove duplicated branches —
also consider moving this spec validation into the webhook near validateFLP so
misconfigs are rejected early instead of being swallowed by builders.

---

161-161: Promote "k8scache" to a named const.

healthPortName, prometheusPortName, profilePortName all have constants above — this one sticks out as a bare string literal. Add k8scachePortName = "k8scache" alongside k8scachePort for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_common_objects.go` at line 161, Promote the
literal "k8scache" to a named constant by adding k8scachePortName = "k8scache"
alongside the existing k8scachePort constant, and replace the literal Name:
"k8scache" with Name: k8scachePortName (or use the constant wherever that port
name is referenced). Update references to the string in the same file so usage
is consistent with healthPortName/prometheusPortName/profilePortName and the
existing k8scachePort symbol.

---

30-30: Consider exposing k8scachePort as configurable (e.g., under Advanced).

Hardcoding a well-known port like 9090 invites collisions with other sidecars or injected tooling (kube-rbac-proxy, Prometheus node-exporter style setups, etc.). Promoting this to an advanced-config field keeps the reserved default but gives operators an escape hatch — which also neatly avoids the whole validatePortConflicts dance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_common_objects.go` at line 30, Replace the
hardcoded k8scachePort = 9090 with a configurable Advanced option: add a field
(e.g., Advanced.K8sCachePort int) to the existing configuration struct and
supply a default of 9090 when loading/constructing the config, then replace uses
of the package-level k8scachePort symbol with reads from that config (falling
back to 9090 if unset); also update any config parsing/CLI flags and
documentation to expose the new Advanced option and keep existing validation (or
simplify validatePortConflicts if desired).

internal/controller/flp/flp_test.go (1)

814-850: Good coverage — consider one small extension.

validatePortConflicts is invoked from both newMonolithBuilder and newTransfoBuilder, but only the monolith path is exercised here. A single mirror case via newTransfoBuilder (or parameterizing the constructor) would guard against someone accidentally dropping the call from one constructor.

Also an ideal candidate for table-driven tests — the five cases repeat the same three-line pattern:

♻️ Table-driven sketch

cases := []struct {
    name   string
    mutate func(*flowslatest.FlowCollectorSpec)
    errSub string
}{
    {"flp port", func(c *flowslatest.FlowCollectorSpec) { c.Processor.Advanced.Port = ptr.To(int32(9090)) }, "flowlogs-pipeline port 9090"},
    {"health port", func(c *flowslatest.FlowCollectorSpec) { c.Processor.Advanced.HealthPort = ptr.To(int32(9090)) }, "health port 9090"},
    {"metrics port", func(c *flowslatest.FlowCollectorSpec) { c.Processor.Metrics.Server.Port = ptr.To(int32(9090)) }, "metrics port 9090"},
    {"profile port", func(c *flowslatest.FlowCollectorSpec) { c.Processor.Advanced.ProfilePort = ptr.To(int32(9090)) }, "profile port 9090"},
    {"ok", func(*flowslatest.FlowCollectorSpec) {}, ""},
}
for _, tc := range cases {
    t.Run(tc.name, func(t *testing.T) { /* ... */ })
}

Based on learnings, testify is an approved framework in this repo, so no need to port to Ginkgo/Gomega just for this.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_test.go` around lines 814 - 850,
TestPortConflictValidation only exercises validatePortConflicts via
newMonolithBuilder; add a mirror set of assertions that call newTransfoBuilder
to ensure validatePortConflicts is enforced there too (or refactor
TestPortConflictValidation into a table-driven loop that runs each test case
against both newMonolithBuilder and newTransfoBuilder). Keep the same mutation
functions and error substring checks (use the existing cases for
Processor.Advanced.Port, HealthPort, Metrics.Server.Port, ProfilePort and the
no-conflict case) and assert the same error messages when invoking
newTransfoBuilder (or both builders) so the coverage is identical for
validatePortConflicts, referencing the existing TestPortConflictValidation,
newMonolithBuilder, newTransfoBuilder and validatePortConflicts symbols.

bundle/manifests/netobserv-operator.clusterserviceversion.yaml (1)

469-486: Missing UI descriptors for consistency.

Other toggles/advanced sections in this CSV use booleanSwitch for enabled flags and hidden/advanced markers for advanced subsections (e.g., consolePlugin.enable, processor.advanced). Consider aligning processor.informers.enabled with booleanSwitch and marking processor.informers.advanced fields with advanced (and a fieldDependency:processor.informers.enabled:true) so the OpenShift form matches existing patterns.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bundle/manifests/netobserv-operator.clusterserviceversion.yaml` around lines
469 - 486, The CSV UI descriptors should mirror existing patterns: change the
descriptor for processor.informers.enabled to use booleanSwitch (like
consolePlugin.enable) and mark the processor.informers.advanced section and its
child fields (processor.informers.advanced.batchSize,
processor.informers.advanced.processorPort,
processor.informers.advanced.resyncInterval,
processor.informers.advanced.sendTimeout,
processor.informers.advanced.updateBufferSize) as advanced/hidden; add a
fieldDependency on processor.informers.enabled:true so the advanced fields only
appear when the Informers toggle is enabled and the OpenShift form behavior
matches other components.

internal/controller/flp/flp_controller_test.go (1)

168-174: Consider asserting informer Deployment/SA lifecycle across Kafka transitions.

The Kafka context verifies the shared informer CRB but doesn't assert the flp-informers Deployment/ServiceAccount persist across the mono→Kafka→mono transitions (Contexts "With Kafka" and "Back without Kafka"). Since this is now a shared, independent deployment, a quick Eventually ... Get(flpKeyInformer, &Deployment{}).Should(Succeed()) in both contexts would guard against accidental teardown on model switch.

Also applies to: 330-337

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_controller_test.go` around lines 168 - 174, Add
assertions in the "With Kafka" and "Back without Kafka" test contexts to verify
the flp-informers Deployment and ServiceAccount persist across the
mono→Kafka→mono transitions: use Eventually with k8sClient.Get and the existing
flpKeyInformer key to ensure Get(ctx, flpKeyInformer, &appsv1.Deployment{}) and
Get(ctx, flpKeyInformer, &corev1.ServiceAccount{}) both succeed (similar to the
existing ClusterRoleBinding check using rb3), so the shared informer
Deployment/SA is not accidentally torn down when switching models.

internal/controller/flp/flp_informer_reconciler.go (2)

66-70: Nit: collapse the nil-check with ptr.Deref.

Minor readability tweak — no behavior change.

Proposed diff

-	// Check if informers are enabled (default: true)
-	enabled := true
-	if desired.Spec.Processor.Informers != nil && desired.Spec.Processor.Informers.Enabled != nil {
-		enabled = *desired.Spec.Processor.Informers.Enabled
-	}
-
-	if !enabled {
+	// Check if informers are enabled (default: true)
+	if desired.Spec.Processor.Informers != nil && !ptr.Deref(desired.Spec.Processor.Informers.Enabled, true) {

Requires k8s.io/utils/ptr import. Skip if the project prefers the explicit form.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_reconciler.go` around lines 66 - 70,
Replace the manual nil-check that initializes the local variable enabled from
desired.Spec.Processor.Informers.Enabled with a single ptr.Deref call: set
enabled := ptr.Deref(desired.Spec.Processor.Informers.Enabled, true) (or the
equivalent expression) so the default true is used when the pointer is nil; add
the import "k8s.io/utils/ptr". Update the block that references
desired.Spec.Processor.Informers.Enabled and remove the explicit if nil { ... }
branch.

---

101-108: SA drift won't be corrected — confirm this is acceptable.

reconcileServiceAccount only creates when missing. If labels/annotations/imagePullSecrets on the SA ever diverge from the builder (e.g., user edit, future builder changes), it won't self-heal until the SA is deleted. The inline comment acknowledges it, so flagging as FYI in case the informers SA gains secrets/annotations later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/flp/flp_informer_reconciler.go` around lines 101 - 108,
reconcileServiceAccount in informerReconciler currently only creates the
ServiceAccount via builder.serviceAccount() when
r.Managed.Exists(r.serviceAccount) is false, so
label/annotation/imagePullSecrets drift won't be corrected; update
reconcileServiceAccount to fetch the existing SA, compare labels, annotations
and imagePullSecrets with builder.serviceAccount() and perform a patch/update
(using r.Update or a strategic merge/patch method) when differences exist while
preserving managed fields, or explicitly document/guard that this resource is
intentionally immutable; reference reconcileServiceAccount, informerReconciler,
builder.serviceAccount(), r.Managed.Exists and r.CreateOwned when making the
change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes